[ML] Implement new rules design #119

dimitris-athanasiou · 2018-06-05T13:49:54Z

The ml-cpp side of elastic/elasticsearch#31110.

Release note: Detectors now support custom rules that enable the user to improve machine learning results by providing some domain-specific knowledge in the form of rule.

tveasey

Left some tidy up suggestions, but basically LGTM.

tveasey · 2018-06-06T10:56:49Z

lib/api/CDetectionRulesJsonParser.cc

        return false;
    }
+
+    for (auto& member : scopeObject.GetObject()) {


nit: looks like it can be const auto&

Tried this but it doesn't compile. I guess I'm using public fields of member and the square brackets operator which probably is not const.

It's because RapidJSON doesn't have begin() and end() functions that return const iterators.

Read downwards from "Just one problem ...." in https://mbevin.wordpress.com/2012/11/14/range-based-for/

In RapidJSON there is this:

#if RAPIDJSON_HAS_CXX11_RANGE_FOR MemberIterator begin() const { return value_.MemberBegin(); } MemberIterator end() const { return value_.MemberEnd(); } #endif

But sadly no corresponding functions that return ConstMemberIterator.

It would be trivial to add - you could submit a PR to RapidJSON if you want to be famous.

I'll hold on to get famous as a rock star instead!

tveasey · 2018-06-06T11:02:00Z

lib/model/CDetectionRule.cc

-    : m_Action(E_FilterResults), m_Conditions(), m_ConditionsConnective(E_Or),
-      m_TargetFieldName(), m_TargetFieldValue() {
-    m_Conditions.reserve(1);
+CDetectionRule::CDetectionRule() : m_Action(E_SkipResult), m_Conditions() {


nit: you can initialise m_Action to E_SkipResult where it is declared in the class body and then keep the auto generated default constructor.

tveasey · 2018-06-06T11:02:49Z

lib/model/CDetectionRule.cc

@@ -51,99 +42,48 @@ bool CDetectionRule::apply(ERuleAction action,
        return false;
    }

-    if (this->isInScope(model, pid, cid) == false) {
+    if (m_Scope.check(model, pid, cid) == false) {
        return false;
    }

    for (std::size_t i = 0; i < m_Conditions.size(); ++i) {


nit: switch to range based for loop.

tveasey · 2018-06-06T11:07:06Z

lib/model/CRuleScope.cc

+bool CRuleScope::check(const CAnomalyDetectorModel& model, std::size_t pid, std::size_t cid) const {
+
+    const CDataGatherer& gatherer = model.dataGatherer();
+    for (auto& scopeField : m_Scope) {


const auto&

tveasey · 2018-06-06T11:13:42Z

lib/model/CRuleScope.cc

+
+std::string CRuleScope::print() const {
+    std::string result{""};
+    if (m_Scope.empty() == false) {


nit: this if is unnecessary, begin can safely be called and you don't enter the loop in this case so no harm done.

tveasey · 2018-06-06T11:14:26Z

lib/model/CRuleScope.cc

+}
+
+void CRuleScope::exclude(std::string field, const core::CPatternSet& filter) {
+    m_Scope.push_back({field, TPatternSetCRef(filter), E_Exclude});


nit: emplace_back

tveasey · 2018-06-06T11:14:38Z

lib/model/CRuleScope.cc

+}
+
+void CRuleScope::include(std::string field, const core::CPatternSet& filter) {
+    m_Scope.push_back({field, TPatternSetCRef(filter), E_Include});


nit: emplace_back

tveasey · 2018-06-06T11:15:16Z

lib/model/CRuleScope.cc

+namespace ml {
+namespace model {
+
+CRuleScope::CRuleScope() : m_Scope() {


nit: use CRuleScope() = default in header.

tveasey · 2018-06-06T11:19:48Z

lib/model/CRuleCondition.cc

-    }
+    std::string result = this->print(m_AppliesTo);
+    result += " " + this->print(m_Condition.s_Op) + " " +
+              core::CStringUtils::typeToString(m_Condition.s_Threshold);
    return result;


nit: naming the result doesn't gain anything in readability IMO, so I'd return a temporary here to guaranty RVO, i.e.

return this->print(m_AppliesTo) + " " + this->print(m_Condition.s_Op) + " " + core::CStringUtils::typeToString(m_Condition.s_Threshold);

The ml-cpp side of the new rules implementation.

dimitris-athanasiou · 2018-06-12T10:44:07Z

@droberts195 @tveasey Could you have another look at this please?

dimitris-athanasiou · 2018-06-12T11:18:01Z

Noting here that I had a go at trying to reuse the state restore traverser paradigm to parse the rules. However, this didn't work out as CJsonStateRestoreTraverser is not handling arrays. It's too risky to fiddle with that just for the sake of parsing the rules so I'm leaving it as is for the time being.

droberts195

LGTM

I left a couple of nits about unnecessary copying. In addition to these there will still be unnecessary copying because core::CTriple doesn't have constructors that take rvalue references. Let's not complicate this PR by changing that here too, but it's worth noting that until all our classes are capable of efficiently consuming moved objects it might be better to stick to passing strings by const reference instead of value.

droberts195 · 2018-06-12T11:06:56Z

lib/model/CRuleScope.cc

+namespace model {
+
+void CRuleScope::include(std::string field, const core::CPatternSet& filter) {
+    m_Scope.emplace_back(field, TPatternSetCRef(filter), E_Include);


Since you're passing field by value you should move it into m_Scope, i.e.:

m_Scope.emplace_back(std::move(field), TPatternSetCRef(filter), E_Include);

Same in exclude immediately below.

droberts195 · 2018-06-12T11:08:16Z

lib/model/CDetectionRule.cc

-void CDetectionRule::addCondition(const CRuleCondition& condition) {
-    m_Conditions.push_back(condition);
+void CDetectionRule::includeScope(std::string field, const core::CPatternSet& filter) {
+    m_Scope.include(field, filter);


Use std::move(field), as it was passed by value and is not used anywhere else.

Same in excludeScope immediately below.

tveasey · 2018-06-12T12:22:21Z

Regarding these cases: I agree, I don't think we should abandon passing by const reference, for consistency if nothing else. However, this is basically LGTM2.

The ml-cpp side of the new rules implementation.

dimitris-athanasiou added v7.0.0 review :ml v6.4.0 >feature labels Jun 5, 2018

dimitris-athanasiou force-pushed the implement-new-rules-design branch from 250a85b to aaa2e04 Compare June 5, 2018 14:12

tveasey reviewed Jun 7, 2018

View reviewed changes

dimitris-athanasiou added 5 commits June 12, 2018 11:42

[ML] Implement new rules design

c3d71bb

The ml-cpp side of the new rules implementation.

Add to changelog

e087b6f

Flattern rule condition object

ec1db1a

Address review comments

2181d41

Remove unncessary condition creation

d1c9b36

dimitris-athanasiou force-pushed the implement-new-rules-design branch from a518172 to d1c9b36 Compare June 12, 2018 10:43

Small formatting issue

d633088

droberts195 approved these changes Jun 12, 2018

View reviewed changes

Pass string as const reference

4d301ab

dimitris-athanasiou merged commit 8ce8abf into elastic:master Jun 12, 2018

dimitris-athanasiou deleted the implement-new-rules-design branch June 12, 2018 13:26

dimitris-athanasiou added a commit that referenced this pull request Jun 12, 2018

[ML] Implement new rules design (#119)

877dccb

The ml-cpp side of the new rules implementation.

sophiec20 added :ml and removed :ml labels Jun 12, 2018

davidkyle mentioned this pull request Dec 19, 2018

[ML] Add include categorical filter type to detector rules #27

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML] Implement new rules design #119

[ML] Implement new rules design #119

dimitris-athanasiou commented Jun 5, 2018 •

edited by lcawl

tveasey left a comment

tveasey Jun 6, 2018

dimitris-athanasiou Jun 7, 2018

droberts195 Jun 7, 2018

dimitris-athanasiou Jun 7, 2018

tveasey Jun 6, 2018

tveasey Jun 6, 2018

tveasey Jun 6, 2018

tveasey Jun 6, 2018

tveasey Jun 6, 2018

tveasey Jun 6, 2018

tveasey Jun 6, 2018

tveasey Jun 6, 2018

dimitris-athanasiou commented Jun 12, 2018

dimitris-athanasiou commented Jun 12, 2018

droberts195 left a comment

droberts195 Jun 12, 2018

droberts195 Jun 12, 2018

tveasey commented Jun 12, 2018

[ML] Implement new rules design #119

[ML] Implement new rules design #119

Conversation

dimitris-athanasiou commented Jun 5, 2018 • edited by lcawl

tveasey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimitris-athanasiou commented Jun 12, 2018

dimitris-athanasiou commented Jun 12, 2018

droberts195 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tveasey commented Jun 12, 2018

dimitris-athanasiou commented Jun 5, 2018 •

edited by lcawl